Conversation
…mprove database readiness checks
There was a problem hiding this comment.
Pull request overview
Updates the GitHub Actions test workflow to run against a pinned PostGIS image and rely on service containers for database lifecycle management.
Changes:
- Add a
postgis/postgis:17-3.5service container to unit and BDD test jobs - Remove
docker composesteps previously used to start/stop the database - Add container healthcheck options for improved readiness signaling
You can also share your feedback on Copilot code review. Take the survey.
| # image: postgis/postgis:latest | ||
| env: | ||
| POSTGRES_PASSWORD: postgres | ||
| POSTGRES_PORT: 5432 |
There was a problem hiding this comment.
POSTGRES_PORT here is set as an environment variable inside the service container, but it does not configure Postgres’ listen port (Postgres listens on 5432 by default) and it is not exported to the job steps. If the tests/app need POSTGRES_PORT, set it under the job-level env: instead; otherwise, remove it to avoid implying it has an effect. (Same issue in the duplicated block in the bdd-tests job.)
| POSTGRES_PORT: 5432 |
| POSTGRES_PASSWORD: postgres | ||
| POSTGRES_PORT: 5432 | ||
| options: >- | ||
| --health-cmd pg_isready |
There was a problem hiding this comment.
The healthcheck pg_isready is more reliable when explicit about user/database (to avoid passing in scenarios where auth/db selection would fail later). Consider using an explicit command (e.g., include -U and -d) consistent with how the app connects.
| --health-cmd pg_isready | |
| --health-cmd pg_isready -U postgres -d postgres |
| services: | ||
| postgis: | ||
| image: postgis/postgis:17-3.5 | ||
| # don't test against latest. be explicit in version being tested to avoid breaking changes | ||
| # image: postgis/postgis:latest | ||
| env: | ||
| POSTGRES_PASSWORD: postgres | ||
| POSTGRES_PORT: 5432 | ||
| options: >- | ||
| --health-cmd pg_isready | ||
| --health-interval 10s | ||
| --health-timeout 5s | ||
| --health-retries 5 | ||
| ports: | ||
| - 5432:5432 |
There was a problem hiding this comment.
The PostGIS service configuration is duplicated in both unit-tests and bdd-tests. To reduce drift, consider factoring this into a reusable workflow (workflow_call) or using YAML anchors (if accepted in this repo’s workflow style) so the service definition is maintained in one place.
Summary
Revert
.github/workflows/tests.ymlfromdocker compose-managed DB startup back to native GitHub Actionsservices.postgis.Why
docker composewas introduced when CI neededpg_cronsupport via a custom DB image build.pg_cronhas since been removed from test setup and is no longer a dependency, so the compose-based path is unnecessary overhead.Changes
services.postgisto bothunit-testsandbdd-testsjobs usingpostgis/postgis:17-3.5.Start database (PostGIS)steps that ran:docker compose build dbdocker compose up -d dbStop databasesteps that ran:docker compose down -vCREATE DATABASE,CREATE EXTENSION IF NOT EXISTS postgis).Impact
Validation